Re-range clone sequences to new group id during promotion#8638
Re-range clone sequences to new group id during promotion#8638codeforall wants to merge 3 commits into
Conversation
citus_promote_clone_and_rebalance promotes a physical streaming replica to a new primary with a freshly-allocated group id, but the clone's sequence objects are byte-for-byte copies of the source primary's and keep the old primary's (groupId << 48) value window. The classical activation path re-ranges sequences per group id via AlterSequenceMinMax; the clone path did not, so the promoted clone and the original primary emitted values from the same window, breaking the global-uniqueness invariant for bigserial/bigint nextval(...) defaults and GENERATED ... AS IDENTITY columns on distributed tables. Add DependentSequenceRangeAdjustCommandList() (mirrors IdentitySequenceDependencyCommandList for nextval/serial sequences) and a promotion-path helper AdjustCloneSequenceRangesForNewGroup() that re-ranges the clone's distributed-table sequences to its new group-id window. The helper runs after SyncNodeMetadataToNodes() over the reused metadata connection, so the clone's corrected local group id is visible when AlterSequenceMinMax executes. Reuses the existing citus_internal.adjust_identity_column_seq_settings UDF; no SQL migration or catalog change. Regression coverage included
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8638 +/- ##
==========================================
- Coverage 88.73% 88.72% -0.02%
==========================================
Files 288 288
Lines 64385 64420 +35
Branches 8109 8112 +3
==========================================
+ Hits 57132 57155 +23
- Misses 4908 4918 +10
- Partials 2345 2347 +2 🚀 New features to boost your workflow:
|
Address review feedback: the clone-promotion sequence re-range only covered distributed tables, but classical add/activate-node re-ranges sequences for every table synced to metadata workers -- reference and Citus local tables included (both support worker-side sequence defaults). AdjustCloneSequenceRangesForNewGroup now iterates AllCitusTableIds() filtered by ShouldSyncTableMetadata(), the same predicate the classical metadata sync uses, so both node-addition paths re-range the same set of sequences. Extend multi_add_node_from_backup_sequences to create distributed, reference and Citus local tables (each with bigserial / bigint DEFAULT nextval / identity columns) BEFORE promotion and assert the promoted clone's sequence windows are disjoint from the source primary's for every table kind. The reference and Citus local assertions collapse without the fix, so they guard the change. Reference-table re-ranging also newly appears in the existing add-node-from- backup tests' NOTICE output.
There was a problem hiding this comment.
Reviewed by GitHub Copilot CLI (Claude Opus 4.8, claude-opus-4.8), directed by @onurctirtir. Static review plus empirical verification in an isolated follower-cluster env (make check-add-backup-node), diffing this head against the pre-fix build.
The core fix is sound:
AllCitusTableIds()filtered byShouldSyncTableMetadata()covers hash-distributed + reference + citus-local + single-shard — exactly what classical add/activate-node syncs (append/range-distributed are excluded there too, since they have no worker-side sequence writes). Expanding this past the original distributed-only version is the right call.- Before/after on a reference table with
bigserial+nextval+GENERATED ALWAYS AS IDENTITYbigint columns: pre-fix the promoted clone kept the source primary's window, so values collided across the two nodes (single>> 48window, duplicate values); with this PR the clone re-ranges to its own group's window (two windows, all values unique). Same result confirmed for a distributed table in a quoted, mixed-case schema. - Ordering is correct (right after
SyncNodeMetadataToNodes), and re-fetching the clone viaFindNodeAnyClusterByNodeIdwhile reusing the coordinated transaction means the adjust UDF reads the clone's new group id — I confirmed the re-ranged window is the new group's, not a stale cached one. That is the crux, since worker sequence values are only globally unique via thegroupId << 48window.
Approving. A few non-blocking comments inline.
| * primary. | ||
| */ | ||
| List * | ||
| DependentSequenceRangeAdjustCommandList(Oid relationId) |
There was a problem hiding this comment.
Minor naming: the siblings here are IdentitySequenceDependencyCommandList / SequenceDependencyCommandList (Sequence...Dependency...), but this one is DependentSequenceRangeAdjustCommandList — the word order is flipped, which hurts grep-ability against the existing family. Consider something like SequenceRangeAdjustCommandList. (The extracted AppendSequenceRangeAdjustCommand helper is a nice DRY cleanup and looks behavior-preserving for the identity path.)
| CREATE SEQUENCE clone_seq_dist_manual AS bigint; | ||
| CREATE TABLE clone_seq_dist ( | ||
| id int, | ||
| bigserial_col bigserial, |
There was a problem hiding this comment.
The re-range command is built with generate_qualified_relation_name + quote_literal_cstr, which is correct — but every table in this test lives in public with plain identifiers, so the quoting path is never actually exercised. Given how often quoted / mixed-case schema + sequence names have caused deparse bugs here, it would be worth adding one case in a non-public, mixed-case schema with a quoted sequence name (e.g. "Edge Schema"."Weird Seq!"). I ran that combination manually and it re-ranges correctly, so this is about locking it in against regressions rather than a bug.
| \c - - - :master_port | ||
| -- The clone's window must differ from the source primary's for every kind. | ||
| SELECT | ||
| :'ref_primary_bigserial_window'::bigint <> :'ref_clone_bigserial_window'::bigint AS ref_bigserial_windows_differ, |
There was a problem hiding this comment.
The distributed and Citus-local sections both assert count(*) = count(DISTINCT col) across the primary+clone rows, but the reference-table section only checks that the windows differ. windows-differ is the stronger structural check, so this is minor — but adding the same uniqueness oracle here would make the three sections symmetric and assert "no duplicate values emitted" directly.
|
@codeforall, didn't intentionally approve, an "agent orchestration error" happened :) Having said that, will probably approve this tomorrow morning once I perform one more pass. Oh we should also add a "DESCRIPTION: " line to have this in release notes. Also, should we backport this to recent minors (as we did for the original implementation) for the managed service? |
2 participants
citus_promote_clone_and_rebalance promotes a physical streaming replica to a new primary with a freshly-allocated group id, but the clone's sequence objects are byte-for-byte copies of the source primary's and keep the old primary's (groupId << 48) value window. The classical activation path re-ranges sequences per group id via AlterSequenceMinMax; the clone path did not, so the promoted clone and the original primary emitted values from the same window, breaking the global-uniqueness invariant for bigserial/bigint nextval(...) defaults and GENERATED ... AS IDENTITY columns on distributed tables.
Add DependentSequenceRangeAdjustCommandList() (mirrors IdentitySequenceDependencyCommandList for nextval/serial sequences) and a promotion-path helper AdjustCloneSequenceRangesForNewGroup() that re-ranges the clone's distributed-table sequences to its new group-id window. The helper runs after SyncNodeMetadataToNodes() over the reused metadata connection, so the clone's corrected local group id is visible when AlterSequenceMinMax executes.
Reuses the existing citus_internal.adjust_identity_column_seq_settings UDF; no SQL migration or catalog change. Regression coverage included